Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MINOR] Migrate RankValue to the package of the common class #265

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

smallzhongfeng
Copy link
Contributor

What changes were proposed in this pull request?

More standardized, and the class can be extended in the future.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

No need.

@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2022

Codecov Report

Merging #265 (a401900) into master (8be8390) will increase coverage by 1.40%.
The diff coverage is 70.00%.

@@             Coverage Diff              @@
##             master     #265      +/-   ##
============================================
+ Coverage     59.70%   61.10%   +1.40%     
- Complexity     1377     1516     +139     
============================================
  Files           166      186      +20     
  Lines          8916     9408     +492     
  Branches        853      918      +65     
============================================
+ Hits           5323     5749     +426     
- Misses         3318     3355      +37     
- Partials        275      304      +29     
Impacted Files Coverage Δ
...fle/coordinator/AbstractSelectStorageStrategy.java 20.00% <ø> (ø)
...e/coordinator/AppBalanceSelectStorageStrategy.java 72.00% <ø> (ø)
...apache/uniffle/coordinator/ApplicationManager.java 83.80% <ø> (ø)
...nator/LowestIOSampleCostSelectStorageStrategy.java 70.90% <ø> (+2.07%) ⬆️
...java/org/apache/uniffle/coordinator/RankValue.java 70.00% <70.00%> (ø)
...ava/org/apache/uniffle/common/RssShuffleUtils.java 0.00% <0.00%> (-95.66%) ⬇️
.../java/org/apache/hadoop/mapreduce/RssMRConfig.java 23.07% <0.00%> (-51.93%) ⬇️
.../apache/uniffle/common/metrics/MetricsManager.java 68.42% <0.00%> (-17.30%) ⬇️
...org/apache/uniffle/common/metrics/GRPCMetrics.java 40.00% <0.00%> (-6.52%) ⬇️
.../java/org/apache/uniffle/common/util/RssUtils.java 63.07% <0.00%> (-6.25%) ⬇️
... and 66 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@smallzhongfeng
Copy link
Contributor Author

Please cc, just a minor change. @jerqi

@jerqi
Copy link
Contributor

jerqi commented Oct 16, 2022

Could you give me a concrete example? Why do we need this pr?

@smallzhongfeng
Copy link
Contributor Author

Because this static class is currently in LowestIOSampleCostSelectStorageStrategy, but AppBalanceSelectStorageStrategy will also use it, and other selection strategy may use it later, it will be common to set it as a normal class. In addition, sorting is required when selecting the optimal storage path. Sorting depends on this object. If other storage needs to rely on this RankValue, and more comparative attributes need to be added, it will be clearer to define these attributes in a separate class. WDYT ? @jerqi

@jerqi
Copy link
Contributor

jerqi commented Oct 17, 2022

Why do we migrate RankValue to the package of the common class if shuffle server and client won't use RankValue.

@smallzhongfeng
Copy link
Contributor Author

I see what you mean, but I think it is necessary to set it as a separate class. Put it under the coordinator package ?

@jerqi
Copy link
Contributor

jerqi commented Oct 17, 2022

I see what you mean, but I think it is necessary to set it as a separate class. Put it under the coordinator package ?

Coordinator package is better.

@jerqi
Copy link
Contributor

jerqi commented Nov 12, 2022

@smallzhongfeng Will you continue this pr?

@smallzhongfeng
Copy link
Contributor Author

@smallzhongfeng Will you continue this pr?

Done, sorry to take so long.

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @smallzhongfeng

@jerqi jerqi merged commit 2df7594 into apache:master Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants